Review Branch: Dummy Hop support for Blinded Payment Path #7
Conversation
Extends the work started in [PR#3917](lightningdevkit#3917) by adding ReceiveAuthKey-based verification for Blinded Payment Paths. This reduces space previously taken by individual ReceiveTlvs and aligns the verification logic with that used for Blinded Message Paths.
Now that we have introduced an alternate mechanism for authentication in the codebase, we can safely remove the now redundant (hmac, nonce) fields from the Payment ReceiveTlvs's while maintaining the security of the onion messages.
PaymentDummyTlv is an empty TLV inserted immediately before the actual ReceiveTlvs in a blinded path. Receivers treat these dummy hops as real hops, which prevents timing-based attacks. Allowing arbitrary dummy hops before the final ReceiveTlvs obscures the recipient's true position in the route and makes it harder for an onlooker to infer the destination, strengthening recipient privacy.
Adds a new constructor for blinded paths that allows specifying the number of dummy hops. This enables users to insert arbitrary hops before the real destination, enhancing privacy by making it harder to infer the sender–receiver distance or identify the final destination. Lays the groundwork for future use of dummy hops in blinded path construction.
WalkthroughIntroduces ReceiveAuthKey-based authentication for blinded payments, replaces UnauthenticatedReceiveTlvs with ReceiveTlvs, updates Router::create_blinded_payment_paths to accept receive_auth_key, adds dummy-hop support in blinded paths, switches decryption to ChaChaDualPoly, propagates an authentication flag in onion payloads, adds a new UnauthenticatedPayload failure reason, removes entropy_source from offers APIs, and updates tests/fuzz accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App as Caller
participant Router
participant Keys as NodeSigner
participant BP as BlindedPaymentPath
App->>Keys: get_receive_auth_key()
Keys-->>App: ReceiveAuthKey
App->>Router: create_blinded_payment_paths(recipient, receive_auth_key, ...)
Router->>BP: new(..., receive_auth_key, ReceiveTlvs, ...)
BP-->>Router: Vec<BlindedPaymentPath>
Router-->>App: Blinded paths
note over BP: Optionally inserts dummy hops before Receive TLV
sequenceDiagram
autonumber
participant Peer
participant Node as Node (msgs)
participant Keys as NodeSigner
participant Onion as Onion Processing
Peer->>Node: HTLC with blinded onion
Node->>Keys: get_receive_auth_key()
Keys-->>Node: ReceiveAuthKey
Node->>Node: Decrypt with ChaChaDualPoly (rho, receive_auth_key)
Node-->>Onion: payload + payment_tlvs_authenticated flag
alt BlindedReceive or TrampolineBlindedReceive
opt payment_tlvs_authenticated == false
Onion-->>Peer: Fail HTLC (UnauthenticatedPayload)
end
Onion-->>Peer: Continue normal validation/processing
else Forward/Dummy
Onion-->>Peer: Handle per variant
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
lightning/src/ln/onion_utils.rs (2)
1683-1690: Additions to LocalHTLCFailureReason are soundNew UnauthenticatedPayload variant and docs look good. Consider explicitly mentioning ReceiveAuthKey-based auth in the doc for clarity.
1731-1731: Wire code mapping aligns with intentMapping UnauthenticatedPayload to the same BADONION | PERM | 24 as InvalidOnionBlinding is correct. Note: From will decode to InvalidOnionBlinding for this code; if intentional, add a brief comment to prevent future confusion.
lightning/src/ln/onion_payment.rs (1)
248-267: New auth gate for blinded receives; verify external error mappingGood addition: early reject unauthenticated receive payloads. Two follow-ups:
- Verify how LocalHTLCFailureReason::UnauthenticatedPayload is surfaced on the wire. If you intend to avoid leaking auth status, consider mapping it to InvalidOnionPayload at the boundary.
- Optional: downgrade log_warn to log_info/log_debug to reduce noise from unauthenticated probes.
lightning/src/ln/msgs.rs (1)
3773-3773: Minor: scope of receive_auth_key.Defined once per decode; OK. If you want to avoid potential “unused” warnings in future refactors, define it inside the blinded-branch.
lightning/src/ln/channelmanager.rs (1)
18746-18750: Avoid redundant shadowing of current_height.If current_height was (or will be) declared earlier as suggested, remove this duplicate let to reduce shadowing.
Apply if earlier declaration exists:
- let current_height: u32 = node[0].node.best_block.read().unwrap().height;lightning/src/blinded_path/payment.rs (2)
697-699: Avoid referencing a temporary&PaymentDummyTlvin the iteratorUsing
&PaymentDummyTlvinsidemaptakes a reference to a temporary. While ZSTs are often const-promoted, it’s clearer and safer to bind once and reuse.Apply:
- let tlvs = intermediate_nodes - .iter() - .map(|node| BlindedPaymentTlvsRef::Forward(&node.tlvs)) - .chain((0..dummy_count).map(|_| BlindedPaymentTlvsRef::Dummy(&PaymentDummyTlv))) - .chain(core::iter::once(BlindedPaymentTlvsRef::Receive(&payee_tlvs))); + let dummy_tlv = PaymentDummyTlv; + let dummy_iter = core::iter::repeat(BlindedPaymentTlvsRef::Dummy(&dummy_tlv)) + .take(dummy_count); + let tlvs = intermediate_nodes + .iter() + .map(|node| BlindedPaymentTlvsRef::Forward(&node.tlvs)) + .chain(dummy_iter) + .chain(core::iter::once(BlindedPaymentTlvsRef::Receive(&payee_tlvs)));Also applies to: 683-705
679-681: Align payment padding round-off with message padding (if intended)
PAYMENT_PADDING_ROUND_OFF = 30differs from message padding. Consider sharing a common constant or documenting why they differ for onion size uniformity.lightning/src/offers/flow.rs (1)
320-356: Routing now threadsReceiveAuthKeycorrectly; consider HTLC min source
- Passing
receive_auth_keyintoRouter::create_blinded_payment_pathsis correct.- Consider deriving
htlc_minimum_msatfrom channel mins or policy instead of hardcoding1.Also applies to: 342-346
fuzz/src/invoice_request_deser.rs (1)
86-87: Derive ReceiveAuthKey instead of using a constant to improve fuzz coverageUse entropy to vary the key so more paths are explored.
- let receive_auth_key = ReceiveAuthKey([41; 32]); + let receive_auth_key = ReceiveAuthKey(entropy_source.get_secure_random_bytes());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
fuzz/src/chanmon_consistency.rs(1 hunks)fuzz/src/full_stack.rs(1 hunks)fuzz/src/invoice_request_deser.rs(5 hunks)fuzz/src/refund_deser.rs(4 hunks)lightning/src/blinded_path/payment.rs(21 hunks)lightning/src/ln/async_payments_tests.rs(1 hunks)lightning/src/ln/blinded_payment_tests.rs(12 hunks)lightning/src/ln/channelmanager.rs(10 hunks)lightning/src/ln/max_payment_path_len_tests.rs(2 hunks)lightning/src/ln/msgs.rs(13 hunks)lightning/src/ln/onion_payment.rs(5 hunks)lightning/src/ln/onion_utils.rs(4 hunks)lightning/src/offers/flow.rs(10 hunks)lightning/src/offers/signer.rs(1 hunks)lightning/src/routing/router.rs(7 hunks)lightning/src/util/test_utils.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
lightning/src/ln/onion_payment.rs (2)
lightning/src/ln/onion_utils.rs (7)
shared_secret(2283-2294)new(783-783)new(2168-2168)new(2172-2174)new(2178-2180)new(2735-2737)new(3953-3955)lightning/src/ln/channel.rs (6)
new(1091-1093)new(1205-1214)new(12825-12876)new(13190-13241)None(11074-11074)None(14118-14118)
lightning/src/ln/max_payment_path_len_tests.rs (2)
lightning/src/blinded_path/payment.rs (1)
new(120-139)lightning/src/offers/flow.rs (1)
new(118-149)
lightning/src/ln/blinded_payment_tests.rs (4)
lightning/src/blinded_path/payment.rs (1)
new(120-139)lightning/src/offers/flow.rs (1)
get_receive_auth_key(192-194)lightning/src/util/test_utils.rs (2)
get_receive_auth_key(1730-1732)get_receive_auth_key(1822-1824)lightning/src/sign/mod.rs (3)
get_receive_auth_key(880-880)get_receive_auth_key(2220-2222)get_receive_auth_key(2393-2395)
lightning/src/ln/msgs.rs (2)
lightning/src/blinded_path/payment.rs (1)
new(120-139)lightning/src/ln/channelmanager.rs (2)
new(3847-3925)new(16114-16135)
lightning/src/routing/router.rs (4)
lightning/src/offers/flow.rs (1)
create_blinded_payment_paths(322-356)fuzz/src/chanmon_consistency.rs (1)
create_blinded_payment_paths(130-136)fuzz/src/full_stack.rs (1)
create_blinded_payment_paths(158-164)lightning/src/util/test_utils.rs (1)
create_blinded_payment_paths(290-308)
lightning/src/blinded_path/payment.rs (2)
lightning/src/blinded_path/message.rs (4)
new_with_dummy_hops(91-119)new(68-85)blinded_hops(171-173)blinded_hops(693-733)lightning/src/ln/msgs.rs (8)
new(2510-2512)new(2545-2547)read(1147-1171)read(1175-1181)read(3345-3356)read(3405-3409)read(3592-3763)read(3770-3912)
lightning/src/offers/flow.rs (3)
lightning/src/routing/router.rs (4)
create_blinded_payment_paths(129-222)create_blinded_payment_paths(247-255)create_blinded_payment_paths(288-292)a(3619-3619)lightning/src/util/test_utils.rs (1)
create_blinded_payment_paths(290-308)lightning/src/ln/channelmanager.rs (2)
usable_channels(5743-5743)test_create_blinded_payment_paths(12930-12942)
lightning/src/util/test_utils.rs (1)
lightning/src/routing/router.rs (14)
first_hops(156-207)first_hops(7886-7886)first_hops(7910-7910)first_hops(8313-8313)first_hops(8323-8323)first_hops(8382-8382)first_hops(8542-8542)first_hops(8622-8622)first_hops(8687-8687)first_hops(8741-8741)first_hops(8861-8861)first_hops(8877-8877)first_hops(8889-8889)first_hops(9033-9033)
🪛 GitHub Actions: Continuous Integration Checks
lightning/src/ln/msgs.rs
[error] 2351-2351: field payment_tlvs_authenticated is never read
[error] 2351-2351: could not compile lightning (lib) due to 1 previous error
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build-tx-sync (ubuntu-latest, stable)
- GitHub Check: benchmark
- GitHub Check: semver-checks
- GitHub Check: build-tx-sync (ubuntu-latest, beta)
- GitHub Check: build-tx-sync (ubuntu-latest, 1.75.0)
- GitHub Check: check_release
- GitHub Check: build-tx-sync (macos-latest, beta)
- GitHub Check: build-tx-sync (ubuntu-latest, 1.75.0)
- GitHub Check: build-tx-sync (macos-latest, stable)
- GitHub Check: build-tx-sync (ubuntu-latest, stable)
- GitHub Check: build-tx-sync (ubuntu-latest, beta)
- GitHub Check: build (macos-latest, stable)
- GitHub Check: benchmark
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build (macos-latest, 1.63.0)
- GitHub Check: semver-checks
- GitHub Check: ext-test
🔇 Additional comments (41)
fuzz/src/full_stack.rs (1)
159-161: Router signature update looks correctFuzzRouter now matches the trait’s new receive_auth_key parameter; unused params are fine here. Nothing else to change.
Please confirm all other Router impls and call sites were updated consistently (build should catch mismatches).
lightning/src/ln/onion_utils.rs (2)
1891-1892: Serialization IDs appended safelyAdding (42, ChannelBalanceOverdrawn) and (43, UnauthenticatedPayload) at the end maintains backward compatibility of the TLV enum. Looks good.
Double‑check no prior release used these indices for different variants.
2053-2054: Debug assertions updated appropriatelyExtending the data-length assertion to UnauthenticatedPayload (32 bytes) matches InvalidOnionBlinding behavior. LGTM.
fuzz/src/refund_deser.rs (1)
71-81: LGTM: fuzz path updated to new auth modelSwitched to ReceiveTlvs and passing ReceiveAuthKey into BlindedPaymentPath::new correctly. No issues.
Also applies to: 99-109
lightning/src/ln/max_payment_path_len_tests.rs (1)
215-235: LGTM: blinded path construction updatedReceiveTlvs and receive_auth_key are correctly threaded into BlindedPaymentPath::new. Path-length/packet-size assertions remain valid.
lightning/src/ln/onion_payment.rs (1)
460-504: Logger deref threading is consistentPassing logger.deref() into decode/create helpers matches the generic L: Deref<Target=Logger> pattern and avoids cloning contexts. Looks good.
lightning/src/ln/msgs.rs (8)
59-59: ChaChaDualPolyReadAdapter import looks correct.Adapter change aligns with adding AAD-based auth; no issues spotted.
3661-3666: Correct: thread receive_auth_key into decrypt; derive rho from ECDH.The switch to ChaChaDualPolyReadAdapter with (rho, receive_auth_key) is sound.
Ensure node_signer.get_receive_auth_key() is stable and non-blocking; if it can fail or rotate, confirm caller expectations.
3676-3683: Good validation: forbid AAD on forward hops.Rejecting used_aad for Forward prevents misrouted/forged use of authenticated TLVs on non-final hops.
3694-3705: Dummy variant decode is fine; carries auth bit forward.No logical issues; field propagation is consistent.
3710-3728: Receive-path propagation looks correct.You pass used_aad into BlindedReceive payload; constraints and bounds checks remain intact.
3826-3844: Trampoline: same forward-hop AAD gating — good.Mirrors non-trampoline path and rejects unexpected authenticated TLVs.
3859-3877: Trampoline receive-path propagation — consistent.Auth flag plumbed through; looks good.
2342-2343: Revert dead_code allowance on payment_tlvs_authenticated
The flag is read in msgs.rs (decode paths) and in onion_payment.rs; no dead_code lint will fire. Remove the proposed#[cfg_attr(not(fuzzing), allow(dead_code))]annotations.Likely an incorrect or invalid review comment.
lightning/src/ln/channelmanager.rs (9)
35-41: Imports align with the payment-context refactor.No issues spotted.
4881-4885: Correctly propagating height and logger.Passing current_height and &*self.logger to create_recv_pending_htlc_info matches the updated signature.
6992-6996: Consistent logger propagation.Forwarding &*self.logger here is consistent with the new API.
14814-14816: Router injected into invoice-request flow looks good.This aligns with creating blinded payment paths in the flow.
17879-17880: Test utilities import is appropriate.TestLogger is used below; no concerns.
18701-18707: Test logger setup LGTM.Creation and later passing of &logger are correct.
18723-18727: Verify current_height declaration order in test.current_height is used here; ensure it’s declared before this call (and not only later at Line 18746). If it wasn’t previously declared, move the declaration above this use; if it was, avoid re-declaring it later.
Suggested adjustment if needed (insert before this call):
+ let current_height: u32 = node[0].node.best_block.read().unwrap().height;
18759-18762: Minor test setup change LGTM.Logger instantiation is fine.
18772-18775: Updated call signature handled correctly.Passing current_height and &logger matches the new API.
lightning/src/routing/router.rs (6)
31-31: Import of ReceiveAuthKey looks goodRequired for the new authenticated blinded paths API.
183-186: Use of saturating_add for max_cltv_expiry is correctPrevents u32 overflow when composing per-hop deltas into the payee constraints. Good safety improvement.
201-205: Passing receive_auth_key into BlindedPaymentPath::newThreading the auth key into blinded path construction aligns with the new authenticated flow; call ordering looks correct in both multi-hop and dummy-hop branches.
Also applies to: 213-216
248-251: FixedRouter signature updateSignature matches the trait change; unreachable/Err behavior preserved.
285-293: Trait docs updated to reflect authentication requirementDoc and signature changes are consistent with the new model.
132-134: Public API change:Router::create_blinded_payment_pathsaddsreceive_auth_key
All internalRouterimpls (DefaultRouter, test utils, fuzz harness) have been updated to include the newreceive_auth_keyparameter. Verify any external Rust or FFI bindings for compatibility. Consider accepting&ReceiveAuthKeyto avoid unnecessary copies.lightning/src/blinded_path/payment.rs (3)
549-556: Reusing TLV type 65539 for Dummy: confirm wire-compat expectationsYou encode Dummy as
(65539, ())and decode withis_dummy. Given the comment about removing prior auth TLV, confirm:
- Older peers won’t misinterpret 65539 in payment contexts.
- We never receive legacy payloads requiring the old field.
Also applies to: 558-563, 595-600
141-150: API for dummy hops looks good; dummy count is bounded
new_with_dummy_hops+ capping viaMAX_DUMMY_HOPS_COUNTis clean and prevents overflow of padding.Also applies to: 688-694
762-767:compute_payinfochange to&ReceiveTlvsis consistentUsing payee constraints from
ReceiveTlvsintegrates with the new auth flow without behavioral regressions.Also applies to: 936-944
lightning/src/offers/flow.rs (4)
942-956: Invoice response flow updated to use new router signature: LGTMSwitch to router-based blinded path creation with
ReceiveTlvsintegrates with the new auth model.Also applies to: 959-967
812-867: Static invoice builder flow updated: LGTMDropping
entropy_sourcehere and relying on router and storedReceiveAuthKeyis coherent.Also applies to: 1642-1661
1298-1402: Async refresh plumbing changes look consistentRouter generics and calls align after the signature changes. No functional regressions spotted.
Also applies to: 1396-1456
312-318: ReceiveAuthKey is Copy, by-value use is safe
ReceiveAuthKey derives Copy/Clone, so returning it from &self simply copies it without moving ownership.lightning/src/util/test_utils.rs (1)
290-304: ReceiveAuthKey threading in TestRouter looks correctSignature and forwarding to inner router align with the updated API. No issues spotted.
lightning/src/ln/async_payments_tests.rs (1)
861-863: Updated TLV access is consistentUsing tlvs.payment_constraints.max_cltv_expiry under Receive variant looks right.
lightning/src/offers/signer.rs (1)
39-51: Reserved HMAC inputs documentedGood defensive note reserving [5;16], [7;16], [8;16]. No behavior change.
fuzz/src/invoice_request_deser.rs (1)
134-143: BlindedPaymentPath::new call matches updated signatureArgument order (recipient, receive_auth_key, payee_tlvs, …) is correct. LGTM.
fuzz/src/chanmon_consistency.rs (1)
130-135: Router stub updated to new signatureSignature matches the new API; no functional impact in fuzz stub.
| let receive_auth_key = node_signer.get_receive_auth_key(); | ||
| let encrypted_control_tlvs = | ||
| &self.inner_path.blinded_hops.get(0).ok_or(())?.encrypted_payload; | ||
| let mut s = Cursor::new(encrypted_control_tlvs); | ||
| let mut reader = FixedLengthReader::new(&mut s, encrypted_control_tlvs.len() as u64); | ||
| match ChaChaPolyReadAdapter::read(&mut reader, rho) { | ||
| Ok(ChaChaPolyReadAdapter { readable, .. }) => Ok((readable, control_tlvs_ss)), | ||
| match ChaChaDualPolyReadAdapter::read(&mut reader, (rho, receive_auth_key.0)) { | ||
| Ok(ChaChaDualPolyReadAdapter { readable, .. }) => Ok((readable, control_tlvs_ss)), | ||
| _ => Err(()), |
There was a problem hiding this comment.
🧩 Analysis chain
Surface AAD auth status or enforce it for receive hops
decrypt_intro_payload discards the used_aad signal returned by ChaChaDualPolyReadAdapter. Ensure unauthenticated receive payloads are rejected downstream (e.g., map to UnauthenticatedPayload) and not just surfaced as a flag.
🏁 Script executed:
#!/bin/bash
# Verify that receive payloads with `payment_tlvs_authenticated == false` are rejected.
rg -n -C2 --type=rust '\bpayment_tlvs_authenticated\b' \
&& rg -n -C2 --type=rust 'UnauthenticatedPayload'Length of output: 5136
Propagate and enforce AAD authentication in decrypt_intro_payload
decrypt_intro_payload currently discards the used_aad flag returned by ChaChaDualPolyReadAdapter::read, allowing unauthenticated control TLVs to be accepted. Modify decrypt_intro_payload to return used_aad (or fail when it’s false) and map failures to UnauthenticatedPayload so downstream code cannot process unauthenticated payloads.
| payment_secret, | ||
| payment_constraints: PaymentConstraints { | ||
| max_cltv_expiry: u32::max_value(), | ||
| htlc_minimum_msat: chan_upd.htlc_minimum_msat, | ||
| }, | ||
| payment_context: PaymentContext::Bolt12Refund(Bolt12RefundContext {}), | ||
| }; | ||
| let nonce = Nonce([43u8; 16]); | ||
| let mut payee_tlvs = payee_tlvs.authenticate(nonce, &expanded_key); | ||
| payee_tlvs.authentication.1 = Nonce([0u8; 16]); | ||
| // Use a mismatched ReceiveAuthKey to force auth failure: | ||
| let mismatched_receive_auth_key = ReceiveAuthKey([0u8; 32]); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Align test expectation with new unauthenticated failure reason
create_recv_pending_htlc_info now returns UnauthenticatedPayload on auth failure. This test still expects InvalidOnionPayload. Update the expectation to UnauthenticatedPayload.
- expect_payment_failed_conditions(
- &nodes[0], payment_hash, true,
- PaymentFailedConditions::new().expected_htlc_error_data(LocalHTLCFailureReason::InvalidOnionPayload, &[]),
- );
+ expect_payment_failed_conditions(
+ &nodes[0], payment_hash, true,
+ PaymentFailedConditions::new().expected_htlc_error_data(LocalHTLCFailureReason::UnauthenticatedPayload, &[]),
+ );If you intend to deliberately coalesce UnauthenticatedPayload to InvalidOnionPayload for external exposure, please confirm the mapping and keep the test as-is; otherwise, apply the diff above. To check usage sites:
Also applies to: 1452-1456
🏁 Script executed:
#!/bin/bash
rg -nP 'UnauthenticatedPayload|InvalidOnionPayload|failure_code\(\)' -C3Length of output: 35327
Replace InvalidOnionPayload with UnauthenticatedPayload in tests
In lightning/src/ln/blinded_payment_tests.rs update both expectations:
- PaymentFailedConditions::new().expected_htlc_error_data(LocalHTLCFailureReason::InvalidOnionPayload, &[])
+ PaymentFailedConditions::new().expected_htlc_error_data(LocalHTLCFailureReason::UnauthenticatedPayload, &[])Apply this change at lines 1410–1419 and again at 1452–1456.
🤖 Prompt for AI Agents
In lightning/src/ln/blinded_payment_tests.rs around lines 1410–1419 and again
around 1452–1456, the tests assert the wrong onion error variant; replace
expectations of InvalidOnionPayload with UnauthenticatedPayload in both places
so the test checks for the UnauthenticatedPayload enum variant instead of
InvalidOnionPayload, updating any pattern matches or assert_eq! calls to use
UnauthenticatedPayload.
1 participant
Summary by CodeRabbit